typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts (1)

729-730: 🧹 Nitpick | 🔵 Trivial

Cache DefaultAzureCredential instead of constructing per request.

Line 729 creates a new credential chain on every LOAD_AZURE_CREDS call. Caching a class-level credential avoids repeated chain setup and matches the existing cached GCP pattern.

#!/bin/bash
set -euo pipefail

# Verify whether DefaultAzureCredential is created per request and whether a cached member exists.
rg -n --type=ts -C2 'new DefaultAzureCredential\(' typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts
rg -n --type=ts -C2 '_azureCredential' typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts

integ-tests/python/test_azure_openai_entra.py (1)

1-88: ⚠️ Potential issue | 🔴 Critical

This is not a pytest integration test and won’t run in the intended integ-test flow.

The file is currently a standalone script (main() + if __name__ == "__main__"), so it contributes no pytest-discovered tests in integ-tests/python. It should be rewritten as pytest test cases (and wired to the uv+maturin integration test path) so CI executes it consistently.

As per coding guidelines, integ-tests/python/**/*.py: "For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".

engine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rs (1)

449-472: 🧹 Nitpick | 🔵 Trivial

Consider guarding against double bearer auth (defensive coding).

If both api_key (line 450) and Entra ID auth (line 470) are configured, both bearer_auth calls execute and the second overwrites the first. While the PR summary states the parser prevents this combination, adding an explicit guard or else branch would make the mutual exclusivity clearer at runtime and prevent subtle bugs if parser validation is bypassed (e.g., dynamic clients).

💡 Suggested defensive refactor

         if let Some(key) = &self.properties.api_key {
             req = req.bearer_auth(key.render(expose_secrets));
-        }
-
-        // Entra ID bearer token acquisition — uses azure_identity on native, JS callback bridge on WASM.
-        {
+        } else if let Some(
+            ResolvedAzureAuthStrategy::EntraId { .. }
+            | ResolvedAzureAuthStrategy::SystemDefault,
+        ) = &self.properties.azure_auth
+        {
+            // Entra ID bearer token acquisition — uses azure_identity on native, JS callback bridge on WASM.
             use internal_llm_client::openai::ResolvedAzureAuthStrategy;
-            if let Some(
-                ResolvedAzureAuthStrategy::EntraId { .. }
-                | ResolvedAzureAuthStrategy::SystemDefault,
-            ) = &self.properties.azure_auth
-            {
-                let azure_auth = super::azure_auth::AzureAuth::get_or_create(
-                    self.properties.azure_auth.as_ref().unwrap(),
-                )
-                .await
-                .map_err(|e| anyhow::anyhow!("Azure Entra ID authentication failed: {e}"))?;
-                let token = azure_auth
-                    .token()
-                    .await
-                    .map_err(|e| anyhow::anyhow!("Azure Entra ID token acquisition failed: {e}"))?;
-                req = req.bearer_auth(&token);
-            }
+            let azure_auth = super::azure_auth::AzureAuth::get_or_create(
+                self.properties.azure_auth.as_ref().unwrap(),
+            )
+            .await
+            .map_err(|e| anyhow::anyhow!("Azure Entra ID authentication failed: {e}"))?;
+            let token = azure_auth
+                .token()
+                .await
+                .map_err(|e| anyhow::anyhow!("Azure Entra ID token acquisition failed: {e}"))?;
+            req = req.bearer_auth(&token);
         }

---

Add a unit test for the bearer-auth branch.

This is the feature's critical path, but the existing tests only set azure_auth: None. A focused unit test asserting that Entra ID / SystemDefault produces an Authorization: Bearer header would catch regressions without requiring live Azure credentials.

As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".

integ-tests/python/azure_openai_entra_example.py (1)

9-19: ⚠️ Potential issue | 🟠 Major

This integration-test file bypasses the repo’s pytest + maturin workflow.

This is implemented as a standalone script (main()), not a pytest test entrypoint, so it won’t participate in the expected integration-test execution path. Please either convert it to a pytest test module or move it outside integ-tests/python if it is intentionally just an example.

As per coding guidelines, "For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".

Also applies to: 34-88

typescript/apps/vscode-ext/src/panels/WebviewPanelHost.ts (1)

726-730: 🧹 Nitpick | 🔵 Trivial

Reuse a cached DefaultAzureCredential instance instead of recreating it per call.

Line 729 creates a new credential each request; this repeats credential-chain initialization and can reduce token-cache effectiveness. This same concern was raised earlier and is still applicable.

#!/bin/bash
# Verify whether DefaultAzureCredential is recreated in handlers
# and whether a class-level cached credential exists.
rg -n --type=ts -C3 '\bDefaultAzureCredential\b|_azureCredential|getToken\('

integ-tests/python/azure_openai_entra_example.py (1)

1-19: ⚠️ Potential issue | 🟠 Major

Convert this script into a pytest-discoverable integration test (or move it out of integ-tests/python).

This file is currently executable-example style (main() + __main__) and won’t run as a standard pytest integration test in the required test workflow.

As per coding guidelines "integ-tests/python/**/*.py: For Python integration tests in integ-tests/python, use uv + maturin to build and pytest to run tests".

Also applies to: 34-88

engine/baml-runtime/src/internal/llm_client/primitive/openai/std_auth.rs (1)

27-37: 🧹 Nitpick | 🔵 Trivial

Add unit tests for cache_key logic.

This is a pure function that maps auth strategies to cache keys and is ideal for unit testing without mocking dependencies. As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible."

🧪 Example test module

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_cache_key_api_key() {
        let strategy = ResolvedAzureAuthStrategy::ApiKey;
        assert_eq!(AzureAuth::cache_key(&strategy), "api_key");
    }

    #[test]
    fn test_cache_key_entra_id() {
        let strategy = ResolvedAzureAuthStrategy::EntraId {
            tenant_id: "tenant-123".to_string(),
            client_id: "client-456".to_string(),
            client_secret: Some("secret".to_string()),
        };
        assert_eq!(AzureAuth::cache_key(&strategy), "tenant-123:client-456");
    }

    #[test]
    fn test_cache_key_system_default() {
        let strategy = ResolvedAzureAuthStrategy::SystemDefault;
        assert_eq!(AzureAuth::cache_key(&strategy), "system_default");
    }
}